Skip to content

Adds logic for role mapping.#37

Merged
mattknatt merged 7 commits into
mainfrom
feature/RoleMapping
Apr 8, 2026
Merged

Adds logic for role mapping.#37
mattknatt merged 7 commits into
mainfrom
feature/RoleMapping

Conversation

@mattknatt

@mattknatt mattknatt commented Apr 7, 2026

Copy link
Copy Markdown
Collaborator

closes #21
closes #19

Summary by CodeRabbit

  • New Features

    • Employee/user mapping UI to create and list mappings (manager-only) with GitHub username and improved header links
    • Case notes store and display richer author info (display name, GitHub username, role)
    • Current user exposed to all views; method-level security enabled
  • Bug Fixes

    • Note creation persists resolved user info and rejects unauthenticated authors
    • Employee creation enforces unique GitHub usernames and preserves deterministic IDs when provided
  • Tests

    • Added tests for current-user resolution, OAuth2 login handling, and author fallback behaviors

@coderabbitai

coderabbitai Bot commented Apr 7, 2026

Copy link
Copy Markdown

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 9f443bab-510f-447f-90c0-2e5851d06ff6

📥 Commits

Reviewing files that changed from the base of the PR and between 465ce3b and d7253ac.

📒 Files selected for processing (1)
  • src/main/java/org/example/projektarendehantering/application/service/CaseService.java

📝 Walkthrough

Walkthrough

Replaced single-string note authors with structured author fields; extended Actor to include displayName and githubUsername; SecurityActorAdapter now resolves/enriches Actor (may consult EmployeeRepository and OAuth2 principal); employee githubUsername added across DTO/entity/mappers/services; UI/controllers/templates/tests updated to support employee mappings and manager-only UI.

Changes

Cohort / File(s) Summary
Case note author model
src/main/java/.../presentation/dto/CaseNoteDTO.java, src/main/java/.../infrastructure/persistence/CaseNoteEntity.java, src/main/java/.../application/service/CaseNoteMapper.java
Replaced single author with authorDisplayName, authorGithubUsername, authorRole; updated DTO/entity fields, constructors, accessors, and mapper conversions; added toEntity(Actor,String) overload that derives author fields and sets createdAt.
Case service / UI flow
src/main/java/.../application/service/CaseService.java, src/main/java/.../presentation/web/UiController.java, src/main/resources/templates/cases/detail.html
addNote now accepts Actor, delegates note creation to mapper, enforces authorization (requireCanRead) and rejects null actor; UiController uses securityActorAdapter.currentUser(); template shows author via displayName → githubUsername → role fallback.
Actor & security resolution
src/main/java/.../common/Actor.java, src/main/java/.../infrastructure/security/SecurityActorAdapter.java, src/test/java/.../infrastructure/security/SecurityActorAdapterTest.java, src/main/java/.../presentation/web/GlobalControllerAdvice.java
Actor record gains displayName and githubUsername; SecurityActorAdapter resolves a name (supports OAuth2 login), generates deterministic userId, looks up EmployeeRepository to populate Actor or falls back to authority→Role mapping; added comprehensive unit tests and a global controller advice providing currentActor.
Employee model & mapping
src/main/java/.../infrastructure/persistence/EmployeeEntity.java, src/main/java/.../application/service/EmployeeMapper.java, src/main/java/.../application/service/EmployeeService.java, src/main/java/.../presentation/dto/EmployeeDTO.java, src/main/java/.../presentation/dto/EmployeeCreateDTO.java, src/main/java/.../infrastructure/persistence/EmployeeRepository.java
Added persistent githubUsername (unique) with getter/setter and constructor change; DTOs updated; mapper sets githubUsername and may derive deterministic UUID from username; service checks findByGithubUsername pre-insert and only assigns random UUID if id null; repository adds findByGithubUsername.
Employee UI
src/main/java/.../presentation/web/EmployeeUiController.java, src/main/resources/templates/employees/list.html, src/main/resources/templates/employees/new.html
New controller and Thymeleaf views to list/create employee mappings with validation and role selection; routes restricted to managers.
Shared header & templates
src/main/resources/templates/fragments/header.html, src/main/resources/templates/cases/detail.html
Header shows “Manage Users” for managers and prefers displayName → githubUsername → role for authenticated user label; case detail updated to use new author fields.
Security config / method security
src/main/java/.../infrastructure/config/SecurityConfig.java
Enabled method-level security via @EnableMethodSecurity.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant UI as UiController
    participant Sec as SecurityActorAdapter
    participant Repo as EmployeeRepository
    participant Service as CaseService
    participant Mapper as CaseNoteMapper
    participant NoteRepo as CaseNoteRepository

    User->>UI: POST /cases/{id}/notes (content)
    UI->>Sec: currentUser()
    Sec->>Repo: findById(deterministicUserIdFromName)
    alt employee found
        Repo-->>Sec: EmployeeEntity (id, displayName, githubUsername, role)
        Sec-->>UI: Actor(userId, role, displayName, githubUsername)
    else not found
        Sec-->>UI: Actor(userId, mappedRole, null, resolvedName)
    end
    UI->>Service: addNote(caseId, content, Actor)
    Service->>Mapper: toEntity(Actor, content)
    Mapper-->>Service: CaseNoteEntity (authorDisplayName/githubUsername/role, createdAt)
    Service->>NoteRepo: save(CaseNoteEntity with case association)
    NoteRepo-->>Service: persisted CaseNote
    Service-->>UI: redirect to case detail
    UI-->>User: HTTP redirect
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Poem

🐰 I hop through mappers, fields, and streams,
swapping lone names for richer dreams.
DisplayName, GitHub, role all aligned,
notes and lists now easier to find.
Tiny paws cheer: "Mappings—refined!"

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check ❓ Inconclusive The title 'Adds logic for role mapping' is partially related but vague. It mentions role mapping generally without specifying the main scope: GitHub OAuth2 user integration with employee mapping, role assignment, and actor-based authorization. Revise the title to be more specific, such as 'Implement role mapping and actor-based authorization for GitHub OAuth2 users' to better reflect the comprehensive nature of the changes across authentication, employee management, and case handling.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Linked Issues check ✅ Passed The changes comprehensively address both linked issues: #21 (role mapping for OAuth2 users via SecurityActorAdapter with GitHub login resolution and EmployeeRepository lookup) and #19 (role assignment via EmployeeUiController with MANAGER-restricted endpoints and default PATIENT role fallback in SecurityActorAdapter).
Out of Scope Changes check ✅ Passed All changes are in-scope: actor-based authorization integration, employee/role management UI, OAuth2 role mapping, case note author tracking, and supporting infrastructure. No unrelated refactoring or unrelated functionality introduced.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feature/RoleMapping

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/main/java/org/example/projektarendehantering/application/service/EmployeeService.java (1)

30-39: ⚠️ Potential issue | 🟠 Major

Address the upsert behavior caused by deterministic UUID generation.

The mapper generates a deterministic UUID from githubUsername (via UUID.nameUUIDFromBytes), which means calling createEmployee twice with the same githubUsername will produce identical entity IDs. The conditional check for null ID won't prevent this, and save() will update the existing record rather than creating a duplicate.

Either document and rename this method to createOrUpdateEmployee if the upsert behavior is intentional, or add an existence check using findByGithubUsername on the repository to prevent accidental overwrites.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@src/main/java/org/example/projektarendehantering/application/service/EmployeeService.java`
around lines 30 - 39, The createEmployee method in EmployeeService currently
upserts because the mapper produces a deterministic UUID from githubUsername; to
fix, either (A) rename and document the method as createOrUpdateEmployee to
reflect intentional upsert behavior and keep current flow, or (B) enforce
create-only semantics by checking
employeeRepository.findByGithubUsername(dto.getGithubUsername()) before saving
and fail/throw an appropriate exception (or return an error) if an existing
record is found; update references to the method accordingly and keep
employeeMapper.toEntity, employeeRepository.save, and requireCanManageEmployees
usages intact.
🧹 Nitpick comments (4)
src/main/java/org/example/projektarendehantering/infrastructure/security/SecurityActorAdapter.java (1)

53-65: Consider logging when falling back to authority-based role resolution.

When no employee mapping exists, the code falls back to Spring Security authorities. This is correct for handling unmapped users, but adding debug/info logging here would help troubleshoot why specific users aren't getting their expected roles from the employee mapping.

♻️ Optional: Add logging for fallback scenario
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
...
+    private static final Logger log = LoggerFactory.getLogger(SecurityActorAdapter.class);
...
         // 2. Fallback to existing logic (checking Spring authorities)
+        log.debug("No employee mapping found for user '{}', falling back to authority-based role resolution", name);
         Role role = Role.PATIENT;
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@src/main/java/org/example/projektarendehantering/infrastructure/security/SecurityActorAdapter.java`
around lines 53 - 65, The fallback branch in SecurityActorAdapter that assigns
Role (Role.PATIENT default and checks authentication.getAuthorities()) should
emit a debug/info log when used so we can trace why employee mapping was absent;
update the method that builds the Actor (the code block using authentication and
Role enum) to log a clear message including the userId and the authorities list
before returning new Actor(userId, role, null, name), using the class logger
(e.g., logger.debug/info) to indicate “falling back to authority-based role
resolution” and the resolved role.
src/main/java/org/example/projektarendehantering/infrastructure/persistence/EmployeeEntity.java (1)

22-23: Consider adding a unique constraint on githubUsername.

Since githubUsername is used to derive a deterministic UUID for employee identification (via UUID.nameUUIDFromBytes), it represents a logical unique identifier. Adding a database constraint ensures data integrity and prevents edge cases where duplicate usernames could be inserted through alternative code paths.

♻️ Proposed fix
+    `@Column`(unique = true)
     private String githubUsername;

Or at the table level:

-@Table(name = "employees")
+@Table(name = "employees", uniqueConstraints = `@UniqueConstraint`(columnNames = "githubUsername"))
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@src/main/java/org/example/projektarendehantering/infrastructure/persistence/EmployeeEntity.java`
around lines 22 - 23, Add a database-level uniqueness constraint for the
githubUsername field in the EmployeeEntity class: update the EmployeeEntity JPA
mapping to declare githubUsername as unique (e.g., `@Column`(unique = true) on the
githubUsername field or add a `@Table`(uniqueConstraints =
`@UniqueConstraint`(columnNames = "githubUsername")) at the class level) and
create a corresponding migration to add the unique index/constraint in the
database so duplicates cannot be inserted through other code paths.
src/main/java/org/example/projektarendehantering/presentation/web/EmployeeUiController.java (1)

37-37: Avoid exposing all enum roles to the UI by default.

At Line 37, Role.values() will auto-expose any newly added enum constant. Prefer an explicit allow-list for assignable roles.

Suggested fix
+import java.util.List;
@@
 public class EmployeeUiController {
+    private static final List<Role> ASSIGNABLE_ROLES = List.of(
+            Role.ADMIN,
+            Role.HANDLER,
+            Role.CASE_OWNER
+    );
@@
-        model.addAttribute("roles", Role.values());
+        model.addAttribute("roles", ASSIGNABLE_ROLES);
@@
-            model.addAttribute("roles", Role.values());
+            model.addAttribute("roles", ASSIGNABLE_ROLES);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@src/main/java/org/example/projektarendehantering/presentation/web/EmployeeUiController.java`
at line 37, The code currently exposes Role.values() in EmployeeUiController
(model.addAttribute("roles", Role.values())), which will automatically include
any future enum constants; replace this with an explicit allow-list of
assignable roles (e.g., define a static collection like ALLOW_ASSIGNABLE_ROLES
or a Role.getAssignableRoles() method that returns only the intended enums) and
use that collection when calling model.addAttribute("roles", ...); update
EmployeeUiController to reference that allow-list instead of Role.values() so
new enum constants are not auto-exposed.
src/main/resources/templates/employees/new.html (1)

28-29: Make the placeholder option non-selectable after selection.

At Line 28, the placeholder is currently a valid option. Mark it disabled/selected to reduce accidental invalid submissions.

Suggested fix
-                <option value="">-- Select Role --</option>
+                <option value="" disabled selected>-- Select Role --</option>
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/main/resources/templates/employees/new.html` around lines 28 - 29, The
placeholder option "-- Select Role --" is currently selectable; update the first
<option> (the placeholder) so it cannot be chosen by adding the disabled and
selected attributes (and optionally hidden) to that element, leaving the th:each
loop that renders roles (th:each="r : ${roles}", th:value="${r}",
th:text="${r}") unchanged so valid roles remain selectable; ensure the
placeholder keeps value="" so any client-side validation will catch an
unselected role.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In
`@src/main/java/org/example/projektarendehantering/application/service/CaseService.java`:
- Around line 48-59: addNote currently creates and saves a CaseNoteEntity
without verifying the actor's permission; update addNote in CaseService to check
authorization on the loaded CaseEntity before creating/saving the note by
calling your authorization component (e.g., AuthorizationService or
PermissionService) with the actor and CaseEntity, and if the check fails throw a
ResponseStatusException(HttpStatus.FORBIDDEN, "Not authorized to add note");
retain the existing actor null handling but ensure null actors are denied unless
your auth service explicitly permits system/anonymous writes.

In
`@src/main/java/org/example/projektarendehantering/presentation/web/EmployeeUiController.java`:
- Around line 34-39: Add method-level authorization so only managers can open
the new employee form: enable method security by annotating SecurityConfig with
`@EnableMethodSecurity`, then protect the controller method
EmployeeUiController.newEmployee with `@PreAuthorize` (e.g.
`@PreAuthorize`("hasRole('MANAGER')")) so non-managers cannot access the GET
/ui/employees/new page; ensure the newEmployee method imports the PreAuthorize
annotation (or, if you prefer not to enable method security, perform an explicit
role check inside EmployeeUiController.newEmployee using SecurityContextHolder
and redirect/deny if the caller lacks the MANAGER role).

In
`@src/main/java/org/example/projektarendehantering/presentation/web/GlobalControllerAdvice.java`:
- Around line 17-23: The currentActor method in GlobalControllerAdvice is
catching all Exceptions which masks real errors; change the catch to only handle
authentication-related failures (e.g., catch
org.springframework.security.core.AuthenticationException or your project's
specific SecurityException) around the call to
securityActorAdapter.currentUser(), return null for those specific exceptions,
and let other exceptions propagate (or rethrow/log and rethrow) so real runtime
errors are not silently swallowed; update the catch clause on currentActor() to
reference the specific exception type(s) instead of Exception and adjust
behavior accordingly.

In `@src/main/resources/templates/employees/new.html`:
- Line 11: Replace the hard-coded href on the "Back to list" anchor with
Thymeleaf URL resolution: change the anchor element that currently uses
href="/ui/employees" to use th:href with the Thymeleaf URL expression (e.g.,
th:href="@{/ui/employees}") while keeping the existing class and text; update
the anchor in the employees/new.html template so the link becomes context-path
aware.

In `@src/main/resources/templates/fragments/header.html`:
- Line 14: The nav visibility check calls currentActor.role().name() without
guarding role(), which can NPE when role is null; update the th:if to explicitly
check the role is non-null (e.g., include a condition like currentActor.role()
!= null or currentActor.role != null) before comparing the name so the anchor's
visibility only evaluates .name() when role exists (refer to the existing th:if
expression that uses currentActor and the role()/name() calls to locate the
check).
- Around line 19-20: The auth label assumes currentActor is non-null and can
throw during template evaluation; update the span.auth-label th:text expression
to use Thymeleaf's null-safe navigation for currentActor (e.g.,
currentActor?.displayName/currentActor?.githubUsername/currentActor?.role) and
provide a final literal fallback like "User" so the whole expression never
dereferences a null currentActor; modify the th:text attribute (on the
span.auth-label) to chain null-safe accesses and a final ?: fallback.

---

Outside diff comments:
In
`@src/main/java/org/example/projektarendehantering/application/service/EmployeeService.java`:
- Around line 30-39: The createEmployee method in EmployeeService currently
upserts because the mapper produces a deterministic UUID from githubUsername; to
fix, either (A) rename and document the method as createOrUpdateEmployee to
reflect intentional upsert behavior and keep current flow, or (B) enforce
create-only semantics by checking
employeeRepository.findByGithubUsername(dto.getGithubUsername()) before saving
and fail/throw an appropriate exception (or return an error) if an existing
record is found; update references to the method accordingly and keep
employeeMapper.toEntity, employeeRepository.save, and requireCanManageEmployees
usages intact.

---

Nitpick comments:
In
`@src/main/java/org/example/projektarendehantering/infrastructure/persistence/EmployeeEntity.java`:
- Around line 22-23: Add a database-level uniqueness constraint for the
githubUsername field in the EmployeeEntity class: update the EmployeeEntity JPA
mapping to declare githubUsername as unique (e.g., `@Column`(unique = true) on the
githubUsername field or add a `@Table`(uniqueConstraints =
`@UniqueConstraint`(columnNames = "githubUsername")) at the class level) and
create a corresponding migration to add the unique index/constraint in the
database so duplicates cannot be inserted through other code paths.

In
`@src/main/java/org/example/projektarendehantering/infrastructure/security/SecurityActorAdapter.java`:
- Around line 53-65: The fallback branch in SecurityActorAdapter that assigns
Role (Role.PATIENT default and checks authentication.getAuthorities()) should
emit a debug/info log when used so we can trace why employee mapping was absent;
update the method that builds the Actor (the code block using authentication and
Role enum) to log a clear message including the userId and the authorities list
before returning new Actor(userId, role, null, name), using the class logger
(e.g., logger.debug/info) to indicate “falling back to authority-based role
resolution” and the resolved role.

In
`@src/main/java/org/example/projektarendehantering/presentation/web/EmployeeUiController.java`:
- Line 37: The code currently exposes Role.values() in EmployeeUiController
(model.addAttribute("roles", Role.values())), which will automatically include
any future enum constants; replace this with an explicit allow-list of
assignable roles (e.g., define a static collection like ALLOW_ASSIGNABLE_ROLES
or a Role.getAssignableRoles() method that returns only the intended enums) and
use that collection when calling model.addAttribute("roles", ...); update
EmployeeUiController to reference that allow-list instead of Role.values() so
new enum constants are not auto-exposed.

In `@src/main/resources/templates/employees/new.html`:
- Around line 28-29: The placeholder option "-- Select Role --" is currently
selectable; update the first <option> (the placeholder) so it cannot be chosen
by adding the disabled and selected attributes (and optionally hidden) to that
element, leaving the th:each loop that renders roles (th:each="r : ${roles}",
th:value="${r}", th:text="${r}") unchanged so valid roles remain selectable;
ensure the placeholder keeps value="" so any client-side validation will catch
an unselected role.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 9435cf40-0b03-41ef-baeb-d6779294dd4c

📥 Commits

Reviewing files that changed from the base of the PR and between cf30359 and 6a75c32.

📒 Files selected for processing (18)
  • src/main/java/org/example/projektarendehantering/application/service/CaseNoteMapper.java
  • src/main/java/org/example/projektarendehantering/application/service/CaseService.java
  • src/main/java/org/example/projektarendehantering/application/service/EmployeeMapper.java
  • src/main/java/org/example/projektarendehantering/application/service/EmployeeService.java
  • src/main/java/org/example/projektarendehantering/common/Actor.java
  • src/main/java/org/example/projektarendehantering/infrastructure/persistence/CaseNoteEntity.java
  • src/main/java/org/example/projektarendehantering/infrastructure/persistence/EmployeeEntity.java
  • src/main/java/org/example/projektarendehantering/infrastructure/security/SecurityActorAdapter.java
  • src/main/java/org/example/projektarendehantering/presentation/dto/CaseNoteDTO.java
  • src/main/java/org/example/projektarendehantering/presentation/dto/EmployeeCreateDTO.java
  • src/main/java/org/example/projektarendehantering/presentation/dto/EmployeeDTO.java
  • src/main/java/org/example/projektarendehantering/presentation/web/EmployeeUiController.java
  • src/main/java/org/example/projektarendehantering/presentation/web/GlobalControllerAdvice.java
  • src/main/java/org/example/projektarendehantering/presentation/web/UiController.java
  • src/main/resources/templates/cases/detail.html
  • src/main/resources/templates/employees/list.html
  • src/main/resources/templates/employees/new.html
  • src/main/resources/templates/fragments/header.html

Comment thread src/main/resources/templates/employees/new.html Outdated
Comment thread src/main/resources/templates/fragments/header.html Outdated
Comment thread src/main/resources/templates/fragments/header.html Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In
`@src/test/java/org/example/projektarendehantering/infrastructure/security/SecurityActorAdapterTest.java`:
- Around line 70-87: The tests (e.g.,
currentUser_whenEmployeeFoundInRepository_shouldReturnActorFromEmployee)
generate UUIDs via username.getBytes() which uses the platform default charset;
update UUID creation to use username.getBytes(StandardCharsets.UTF_8) to match
production code and avoid platform-dependent failures, and add the import for
java.nio.charset.StandardCharsets; apply this change for all UUID
initializations in SecurityActorAdapterTest (the occurrences where
UUID.nameUUIDFromBytes(...) is called).
- Around line 30-161: Add a test exercising the OAuth2AuthenticationToken branch
in SecurityActorAdapter.currentUser: mock an OAuth2AuthenticationToken (or mock
Authentication and set instanceof behavior) and an OAuth2User that returns a
"login" attribute (e.g., "oauth-user") for getAttribute("login"), set
SecurityContext.getAuthentication() to that token, stub
employeeRepository.findById(UUID.nameUUIDFromBytes(login.getBytes())) to return
Optional.empty() or an EmployeeEntity as needed, and stub
authentication.getAuthorities() for role fallback checks; then call
securityActorAdapter.currentUser() and assert the Actor's githubUsername, userId
(derived from the login), role, and displayName as in the other tests.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 4876c4e1-e2a6-48ac-a994-5e4c54d9f46d

📥 Commits

Reviewing files that changed from the base of the PR and between 6a75c32 and c2d95a1.

📒 Files selected for processing (1)
  • src/test/java/org/example/projektarendehantering/infrastructure/security/SecurityActorAdapterTest.java

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/main/java/org/example/projektarendehantering/infrastructure/config/SecurityConfig.java (1)

16-28: ⚠️ Potential issue | 🟠 Major

OAuth2 role mapping is still missing from the login flow.

The oauth2Login() block (lines 26–28) does not configure a GrantedAuthoritiesMapper via userInfoEndpoint(). GitHub OAuth2 users will not be assigned the required ADMIN/CASE_OWNER roles from issue #21. The in-memory userDetailsService() only handles form login and cannot apply GitHub role mapping.

🔧 Suggested patch
+import org.springframework.security.core.authority.SimpleGrantedAuthority;
+import org.springframework.security.core.authority.mapping.GrantedAuthoritiesMapper;
+import org.springframework.security.oauth2.client.userinfo.OAuth2UserAuthority;
+
+import java.util.Arrays;
+import java.util.Set;
+import java.util.stream.Collectors;
@@
-    public SecurityFilterChain securityFilterChain(HttpSecurity http) throws Exception {
+    public SecurityFilterChain securityFilterChain(HttpSecurity http,
+                                                   GrantedAuthoritiesMapper githubAuthoritiesMapper) throws Exception {
         http
             .oauth2Login(oauth2 -> oauth2
+                .userInfoEndpoint(userInfo -> userInfo.userAuthoritiesMapper(githubAuthoritiesMapper))
                 .defaultSuccessUrl("/", true)
             )
@@
     }
+
+    `@Bean`
+    public GrantedAuthoritiesMapper githubAuthoritiesMapper() {
+        Set<String> admins = Arrays.stream(
+                System.getenv().getOrDefault("GITHUB_ADMIN_USERNAMES", "").split(","))
+            .map(String::trim)
+            .filter(s -> !s.isBlank())
+            .collect(Collectors.toSet());
+
+        return authorities -> authorities.stream()
+            .filter(OAuth2UserAuthority.class::isInstance)
+            .map(OAuth2UserAuthority.class::cast)
+            .findFirst()
+            .map(oauthAuthority -> {
+                String username = String.valueOf(oauthAuthority.getAttributes().get("login"));
+                if (admins.contains(username)) {
+                    return Set.of(new SimpleGrantedAuthority("ROLE_ADMIN"));
+                }
+                return Set.of(new SimpleGrantedAuthority("ROLE_CASE_OWNER"));
+            })
+            .orElse(authorities);
+    }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@src/main/java/org/example/projektarendehantering/infrastructure/config/SecurityConfig.java`
around lines 16 - 28, The oauth2Login() block in
SecurityConfig.securityFilterChain(HttpSecurity) lacks mapping of OAuth2 user
authorities to application roles, so GitHub logins won't receive
ADMIN/CASE_OWNER roles; add a GrantedAuthoritiesMapper and wire it into the
OAuth2 flow via oauth2Login().userInfoEndpoint().userAuthoritiesMapper(...).
Implement a mapper method (e.g., grantedAuthoritiesMapper()) that examines
OAuth2UserAuthority or user attributes (org membership, team, or a configured
claim) and returns a Set of SimpleGrantedAuthority with "ROLE_ADMIN" /
"ROLE_CASE_OWNER" as needed, expose it as a bean or private method, and
reference that mapper from securityFilterChain to ensure OAuth2 users get the
required roles (note: userDetailsService() only covers form login).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In
`@src/main/java/org/example/projektarendehantering/infrastructure/config/SecurityConfig.java`:
- Around line 16-28: The oauth2Login() block in
SecurityConfig.securityFilterChain(HttpSecurity) lacks mapping of OAuth2 user
authorities to application roles, so GitHub logins won't receive
ADMIN/CASE_OWNER roles; add a GrantedAuthoritiesMapper and wire it into the
OAuth2 flow via oauth2Login().userInfoEndpoint().userAuthoritiesMapper(...).
Implement a mapper method (e.g., grantedAuthoritiesMapper()) that examines
OAuth2UserAuthority or user attributes (org membership, team, or a configured
claim) and returns a Set of SimpleGrantedAuthority with "ROLE_ADMIN" /
"ROLE_CASE_OWNER" as needed, expose it as a bean or private method, and
reference that mapper from securityFilterChain to ensure OAuth2 users get the
required roles (note: userDetailsService() only covers form login).

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: caf7cdf6-637c-4672-af5b-2429d7c6b8c8

📥 Commits

Reviewing files that changed from the base of the PR and between c2d95a1 and 85dd8f0.

📒 Files selected for processing (9)
  • src/main/java/org/example/projektarendehantering/application/service/EmployeeService.java
  • src/main/java/org/example/projektarendehantering/infrastructure/config/SecurityConfig.java
  • src/main/java/org/example/projektarendehantering/infrastructure/persistence/EmployeeEntity.java
  • src/main/java/org/example/projektarendehantering/infrastructure/persistence/EmployeeRepository.java
  • src/main/java/org/example/projektarendehantering/presentation/web/EmployeeUiController.java
  • src/main/java/org/example/projektarendehantering/presentation/web/GlobalControllerAdvice.java
  • src/main/resources/templates/employees/new.html
  • src/main/resources/templates/fragments/header.html
  • src/test/java/org/example/projektarendehantering/infrastructure/security/SecurityActorAdapterTest.java
✅ Files skipped from review due to trivial changes (2)
  • src/main/java/org/example/projektarendehantering/infrastructure/persistence/EmployeeRepository.java
  • src/main/resources/templates/employees/new.html
🚧 Files skipped from review as they are similar to previous changes (4)
  • src/main/java/org/example/projektarendehantering/application/service/EmployeeService.java
  • src/main/java/org/example/projektarendehantering/infrastructure/persistence/EmployeeEntity.java
  • src/main/java/org/example/projektarendehantering/presentation/web/EmployeeUiController.java
  • src/main/resources/templates/fragments/header.html

… `CaseNoteMapper` to simplify note creation logic.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (1)
src/main/java/org/example/projektarendehantering/application/service/CaseService.java (1)

54-60: ⚠️ Potential issue | 🔴 Critical

Authorize the case before saving the note.

Any non-null actor can still append a note to any case ID here because the loaded CaseEntity is never checked against the caller. At minimum, gate this path with requireCanRead(actor, caseEntity) before the save.

🔒 Proposed fix
 CaseEntity caseEntity = caseRepository.findById(caseId)
         .orElseThrow(() -> new ResponseStatusException(HttpStatus.NOT_FOUND, "Case not found"));
+requireCanRead(actor, caseEntity);
 
 CaseNoteEntity note = caseNoteMapper.toEntity(actor, content);
 note.setCaseEntity(caseEntity);
 
 caseNoteRepository.save(note);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@src/main/java/org/example/projektarendehantering/application/service/CaseService.java`
around lines 54 - 60, The code creates a CaseEntity via
caseRepository.findById(...) and saves a CaseNoteEntity without verifying the
caller; before converting and saving the note, call requireCanRead(actor,
caseEntity) (or the appropriate authorization method) to authorize the actor
against the loaded CaseEntity, e.g. retrieve the CaseEntity, invoke
requireCanRead(actor, caseEntity), then proceed with
caseNoteMapper.toEntity(actor, content), note.setCaseEntity(caseEntity) and
caseNoteRepository.save(note).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In
`@src/main/java/org/example/projektarendehantering/application/service/CaseService.java`:
- Around line 54-60: The code creates a CaseEntity via
caseRepository.findById(...) and saves a CaseNoteEntity without verifying the
caller; before converting and saving the note, call requireCanRead(actor,
caseEntity) (or the appropriate authorization method) to authorize the actor
against the loaded CaseEntity, e.g. retrieve the CaseEntity, invoke
requireCanRead(actor, caseEntity), then proceed with
caseNoteMapper.toEntity(actor, content), note.setCaseEntity(caseEntity) and
caseNoteRepository.save(note).

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 2b15c80a-9bca-42b5-8dfe-79210b2aacf2

📥 Commits

Reviewing files that changed from the base of the PR and between 85dd8f0 and 465ce3b.

📒 Files selected for processing (3)
  • src/main/java/org/example/projektarendehantering/application/service/CaseNoteMapper.java
  • src/main/java/org/example/projektarendehantering/application/service/CaseService.java
  • src/main/java/org/example/projektarendehantering/presentation/web/EmployeeUiController.java
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/main/java/org/example/projektarendehantering/presentation/web/EmployeeUiController.java

@mattknatt
mattknatt merged commit 7416cec into main Apr 8, 2026
2 checks passed
@mattknatt
mattknatt deleted the feature/RoleMapping branch April 8, 2026 09:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant